-
Notifications
You must be signed in to change notification settings - Fork 610
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Separate label/value for each suggestion on the list. #16852
Separate label/value for each suggestion on the list. #16852
Conversation
It is now possible to show a suggestion label in completer, but insert a suggestion value into the input instead. In addition to String as before, each list item now can also be: - a `{ label, value }` Object - a `[ label, value ]` Array To show full country name in completer, but insert country code into the input you can use these items: - `{ label: "United States", value: "US" }` - `[ "United States", "US" ]` Despite this data format change, old code will continue to work as before. This is taken care by `Suggestion()`. It uses `label` property automatically when string is expected anywhere in the API. In addition to default support for String/Object/Array items, we also add `data` method, which can be used to support any additional custom item formats and to generate data dynamically, as in changed Email example. The only thing you need to do in this case is to return item in any of String/Array/Object formats supported by default.
This PR is pretty much exactly what I was talking about! Thank you @vlazar! |
this.label = o.label; | ||
this.value = o.value; | ||
} | ||
Suggestion.prototype = new String; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to use Object.create(String.prototype)
here I think, that way the constructor is not called pointlessly. But that’s minor (and more of a general best practice).
Also, I’m not sure if this is in general a good way to subclass builtins, since builtins are special in a number of ways. E.g. I suspect (haven't tested it) that length
will not work properly so we might need to define our own getter. Check this out, notably the section about __proto__
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to Object.create(String.prototype)
and provided correct length
in 61666cf
Sure, length is not live updated, but then we can also modify value
and label
while pretending to be just a String.
So should we care about not being exactly the same as String here?
I believe it's not fully doable in ES5.
Will we eventually remove Suggestion in 2.0 or do you want to keep it forever?
I'll check the link you provided and explore other solutions as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another one based on __proto__
2471773
Browser support http://kangax.github.io/compat-table/es6/#test-Object.prototype.__proto__
We should also probably wrap with |
Neither |
@LeaVerou See my 2 versions of Suggestion changed based on your comments And my comments on PR as well. Let me know if we go with one of these implementations, but don't merge PR please yet. I'll make a clean version instead once we agree. Also we probably need to add the section about the new |
Created LGTM label. Please add it to this PR, once it is good to merge. |
Let's go with 61666cf for now, we can always change it to use |
Apart from my comment about |
FYI, this won't work: var sg = new Suggestion("Hello");
sg[0]; // undefined With |
@LeaVerou If my previous comment changes your decision, please just remove LGTM label. Otherwise I'll go with cleanup, docs and tests. |
Can we do both? (we need to check if |
Here is how it can look then: var o = {};
o.__proto__ = Array.prototype;
var protoSupported = o instanceof Array;
function Suggestion(data) {
var o = Array.isArray(data)
? { label: data[0], value: data[1] }
: typeof data === "object" && "label" in data && "value" in data ? data : { label: data, value: data };
var me;
if (protoSupported) {
me = new String(o.label);
me.__proto__ = Suggestion.prototype;
} else {
me = this;
}
me.label = o.label;
me.value = o.value;
return me;
}
Suggestion.prototype = Object.create(String.prototype);
if (!protoSupported) {
Suggestion.prototype.toString = Suggestion.prototype.valueOf = function () {
return this.label;
};
Object.defineProperty(Suggestion.prototype, "length", {
get: function() { return this.label.length; }
});
} Doesn't if feel like a too clumsy solution? Also in case without If we loosen your requirement for function suggestion(data) {
var o = Array.isArray(data)
? { label: data[0], value: data[1] }
: typeof data === "object" && "label" in data && "value" in data ? data : { label: data, value: data };
var str = new String(o.label);
str.label = o.label;
str.value = o.value;
// Uncaught TypeError: Cannot redefine property: length(…)
// Object.defineProperty(str, "length", {
// get: function() { return this.label.length; }
// });
return str;
} This is the simplest of all solutions. It's concise and it will work in all browsers without any We can use this for |
@vlazar, I think we’ve discussed this before. We are not going to stop letting people use simple strings because it makes our code easier. So far Awesomplete has supported only simple strings and people do use it, meaning there is a ton of use cases where label == value. Most, I would say. Enabling people to do more complex things (like having label != value) does not mean we have to complicate everybody else's code. The possibility should be there for when they need it, but it shouldn't get in the way when they don't. Please don't see those who want label = value as this rare edge case that we don't care much about and are only supporting for a bit while they're transitioning to this new superior API. These are all our current users. Everyone who uses Awesomplete today is using it in cases where label = value. These use cases are our PRIMARY priority. It's the label != value that is an on the side thing. I hate having a Suggestion class or function just as much as you. If we can think of something better now or in the future, that would be fantastic! But if we can't think of something better, delegating the problem to our users and complicating their code even one iota is NOT a solution. That is not how good APIs are designed. Usability should be a priority. There are tons of other autocomplete widgets that do way more than Awesomplete. The reason people embraced Awesomplete is not because it has the most features, but because it's small, usable and extensible. Let's try to keep these traits. Regarding the code, yes, the first one is indeed too clumsy. However, we cannot give up the length property. Not to mention that in your second example, if a user updates Also, once we finalize the design, it might be a good idea to have a few people test this out in real use cases before we deploy. Maybe we should first add it as a separate branch and ping those who requested it to try it out? Just a thought. What do you think? |
I'll go back to the original subclassing, with a dynamic Just to make sure we are on the same page. This is an explicit API I was talking about: filter: function (suggestion, input) {
return RegExp($.regExpEscape(input.trim()), "i").test(suggestion); // 1.x
return RegExp($.regExpEscape(input.trim()), "i").test(suggestion.label); // 2.x
} Sorry, if we've already discussed this and I've missed your point. Just wanted to explore all possible options, as I don't like already discussed ones too much. The part that bothers me most, is not the clumsiness, but that not everything will work, while we are trying to do absolutely no changes in API. Sad, it's only an IE10 support that holds us back. Otherwise |
I was planning to make sure everything, including all the old examples and suggested features, will work before accepting this PR as LGTM by myself, but sure, we can ask others to check this out as well. Don't know how many will bother to check it though. It might have been easier with continuous npm releases. |
Since we are going to invite others to test this, I've also added support for list with separate label/value via |
Wow, that's a lot of work, thanks! People will not manually merge this PR to test the new functionality out and might not realize they can get the full file from your repo. We need to add a branch so that they can just download a file and use it, then give them a link. The less work they have to do and the less knowledge of git(hub) they need to try it out, the more people actually will try it. Also, I'm more interested in the usability of the JS API right now. If people use it with |
There is a short summary in bold in PR description with a link to JS file. |
Oh cool, I hadn't noticed you edited the PR description :) |
@nixprosoft You need to provide custom |
Thank you both for the great work. A short feedback on the changes: If have tested the script today in a simple use case where I work with the value when the awesomplete-selectcomplete event is fired and everything works as expected. |
@vlazar Thanks! Understood clearly! |
@webguerilla Thanks. You probably meant There was a discussion with @LeaVerou about having item's data accessible in events like |
@vlazar I actually did use |
Closing in favor of #16866 |
This is an attempt to make a cleaner version of #16851
To all interested in testing it here is the link to raw version https://raw.githubusercontent.com/vlazar/awesomplete/feature/separate-label-and-value/awesomplete.js
Earlier
filter()
,sort()
,item()
andreplace()
were receiving list items as strings. Now they are receiving item as an object with alabel
and avalue
. But old API still works via privateSuggestion()
wrapper which pretends to be a String.Known issue with current implementation:
suggestion[0]
won't work, everything other you expect from String should work fine.It is now possible to show a suggestion label in completer, but insert a suggestion value into the input instead.
In addition to String as before, each list item now can also be:
{ label, value }
Object[ label, value ]
ArrayTo show full country name in completer, but insert country code into the input you can use these items:
{ label: "United States", value: "US" }
[ "United States", "US" ]
Despite this data format change, old code will continue to work as before. This is taken care by
Suggestion()
. It useslabel
property automatically when string is expected anywhere in the API.In addition to default support for String/Object/Array items, we also add
data
method, which can be used to support any additional custom item formats and to generate data dynamically, as in changed Email example. The only thing you need to do in this case is to return item in any of String/Array/Object formats supported by default.It's also possible now to have a different label/value via
<datalist>
element with<option label="United States" value="US">
,<option value="US">United States</option>
or<option label="United States">US</option>
.